Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tapchannel: send+recv chunks of the input proof to stay under max msg limit #1259

Merged
merged 4 commits into from
Dec 19, 2024
Merged

tapchannel: send+recv chunks of the input proof to stay under max msg limit #1259

merged 4 commits into from
Dec 19, 2024

Conversation

Roasbeef
Copy link
Member

In this PR, we start to send+recv chunks of the input proofs. This
ensures that if a suffix proof is larger than the lnwire message size,
then we'll be able to still send+recv it

Fixes #1250

@Roasbeef Roasbeef requested review from a team, ffranr and GeorgeTsagk and removed request for a team December 17, 2024 18:41
@guggero guggero requested review from guggero and removed request for ffranr December 17, 2024 18:56
@dstadulis dstadulis added this to the v0.5 milestone Dec 17, 2024
@Roasbeef Roasbeef force-pushed the chunked-funding-proofs branch from 4e09c15 to d0ab7dc Compare December 17, 2024 20:55
@coveralls
Copy link

coveralls commented Dec 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 12411616736

Details

  • 129 of 228 (56.58%) changed or added relevant lines in 3 files are covered.
  • 37 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+0.04%) to 40.73%
Changes Missing Coverage Covered Lines Changed/Added Lines %
tapchannelmsg/wire_msgs.go 114 130 87.69%
tapchannel/aux_funding_controller.go 0 83 0.0%
Files with Coverage Reduction New Missed Lines %
tapchannel/aux_funding_controller.go 1 0.0%
tappsbt/create.go 2 53.22%
tapgarden/planter.go 2 74.12%
tapchannel/aux_leaf_signer.go 3 43.43%
commitment/tap.go 3 83.64%
universe/interface.go 4 51.95%
tapdb/multiverse.go 6 68.21%
asset/asset.go 6 80.92%
asset/mock.go 10 91.08%
Totals Coverage Status
Change from base Build 12379489363: 0.04%
Covered Lines: 25952
Relevant Lines: 63717
💛 - Coveralls

guggero
guggero reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thanks for the quick bug fix.

What's the backward compatibility here? I assume channel funding would just result in an error (or timeout) if an rc3 client attempts to open with an rc2 one?
Sounds good to me, just thinking aloud.

tapchannelmsg/wire_msgs.go
@@ -237,6 +319,8 @@ type AssetFundingCreated struct {
// funding output to be able to create the aux funding+commitment
// blobs.
FundingOutputs tlv.RecordT[tlv.TlvType1, AssetOutputListRecord]

// TODO(roasbeef): need to chunk this??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say no, or not yet. We currently only allow 3 asset IDs to be committed to a channel (maxNumAssetIDs). And because the proofs for this output should be quite small (only one on-chain funding output and a change output), and only proof suffixes, I doubt we'd get up to 64k.

tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
tapchannel/aux_funding_controller.go Show resolved Hide resolved
tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
tapchannelmsg/wire_msgs.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member Author

What's the backward compatibility here? I assume channel funding would just result in an error (or timeout) if an rc3 client attempts to open with an rc2 one?

Yeah if rc2 opens with rc3, there'd be an error/timeout as the verifier now relies on reading that Last bool to decide if it should combine the chunks and decode into the entire proof. If we want to, we can add more backwards compat if we assume a chunk size. So with this approach, if the chunk we get is below the chunk size, then we assume that it's the entire proof as proceed as normal.

@guggero
Copy link
Member

guggero commented Dec 18, 2024

Btw, the litd itest failure seems to be a flake, verified locally that all itests still pass with this.

@Roasbeef Roasbeef force-pushed the chunked-funding-proofs branch from d0ab7dc to 3ce6641 Compare December 18, 2024 16:15
@Roasbeef
Copy link
Member Author

Pushed up a new version.

GeorgeTsagk
GeorgeTsagk reviewed Dec 18, 2024
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

tapchannelmsg/wire_msgs.go Show resolved Hide resolved
tapchannel/aux_funding_controller.go Outdated Show resolved Hide resolved
@GeorgeTsagk
Copy link
Member

GeorgeTsagk commented Dec 18, 2024 •
edited
Loading

Btw, the litd itest failure seems to be a flake, verified locally that all itests still pass with this.

We could add itest coverage including an input proof that is > 60k bytes

guggero
guggero approved these changes Dec 18, 2024
View reviewed changes
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending other review comments 🎉

@Roasbeef
asset: update GenesisGen to always generate a valid Genesis
ed644c2 
Before this change, at times the Tag would be too large, or the asset
type an unknown value.
@Roasbeef
tapchannelmsg: add new ProofChunk record type
609d5e0 
This will be used to chunk up proofs that are too large into smaller
chunks for reassembly on the other side.
@Roasbeef
tapchannelmsg: add routines for chunking and unchunking a proof file
f8fbf20 
In this commit, we add functions to chunk and unchunk a proof file.
Given a chunk size, we'll split up the proof into smaller components,
along with a hash digest for authentication verification and tracking.
On the other side, we'll collect all the chunks, check the digest, and
finally decode the chunk itself.

Property based tests are also added for both the positive and the
negative cases.
@Roasbeef
tapchannelmsg: send+recv chunks of the input proofs
afce88e 
In this commit, we start to send+recv chunks of the input proofs. This
ensures that if a suffix proof is larger than the `lnwire` message size,
then we'll be able to still send+recv it.
@Roasbeef Roasbeef force-pushed the chunked-funding-proofs branch from 3ce6641 to afce88e Compare December 19, 2024 11:16
@Roasbeef Roasbeef requested a review from GeorgeTsagk December 19, 2024 11:17
GeorgeTsagk
GeorgeTsagk approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

良さそうだ ✅

@Roasbeef Roasbeef merged commit 34a3aaf into lightninglabs:main Dec 19, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GeorgeTsagk GeorgeTsagk GeorgeTsagk approved these changes

@guggero guggero guggero approved these changes

Assignees

@Roasbeef Roasbeef

Labels
bug fix proofs tap-channels
Projects
Taproot-Assets Project Board
Status: ✅ Done
Milestone
v0.5
Development

Successfully merging this pull request may close these issues.

[bug]: Unable to fundchannel - message payload is too large
5 participants
@Roasbeef @coveralls @guggero @GeorgeTsagk @dstadulis